Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tutorial step 6 - Uplift to latest ink! and Openbrush #17

Open
wants to merge 2 commits into
base: tutorial/events
Choose a base branch
from

Conversation

bobo-k2
Copy link
Collaborator

@bobo-k2 bobo-k2 commented Jul 31, 2023

No description provided.

@bobo-k2 bobo-k2 requested a review from Maar-io July 31, 2023 07:52
@@ -67,9 +62,44 @@ pub mod shiden34 {
});
}

impl payable_mint_pkg::impls::payable_mint::payable_mint::Internal for Shiden34 {}
impl payable_mint::Internal for Shiden34 {}

impl payable_mint::PayableMintImpl for Shiden34 {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, atm, otherwise the code won't compile

impl PayableMint for Shiden34 {
#[ink(message, payable)]
fn mint(&mut self, to: AccountId, mint_amount: u64) -> Result<(), PSP34Error> {
payable_mint::PayableMintImpl::mint(self, to, mint_amount)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think this is a good way. You have 2 implementation of basically the same trait
It should be as simple as


impl payable_mint::PayableMint for Shiden34Contract {}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be ideal. I will investigate how to do that with the latest Openbrush

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants